Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

404 Page handling w/ issue form (#362) #365

Merged
merged 4 commits into from
Apr 2, 2024

Conversation

gabrielhsdev
Copy link

@gabrielhsdev gabrielhsdev commented Mar 30, 2024

This pull request introduces the updateGitHref() function. It is responsible for dynamically updating a GitHub issue link with relevant information.

Referrer URL Handling: It's worth noting that accessing document.referrer might result in an empty value if the user navigated directly to the page or if the browser's privacy settings restrict referrer information. The function handles such scenarios, providing a default value to prevent errors.

Issue: #362 from @tfoote

Copy link
Member

@tfoote tfoote left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks this looks like it's pretty close to what we need. Can you update the text when the results are not provided such that users won't keep making the same mistakes.

It might be that the undetected value should be blank and the text descriptions should be more verbose saying "If this isn't autodetected it means you have to fill it in yourself.

And with the required: true then the form won't submit with the unfilled URLs.

404.html Outdated Show resolved Hide resolved
Copy link
Member

@tfoote tfoote left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks this looks reasonable. I don't have a good way to validate it without merging so I'll deploy it and we may need to do a followup.

Copy link
Member

@tfoote tfoote left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry looking closer you put the message into the placeholder spot. Which I think will then pass validation because the content is not empty and we'll get a bunch of issues filed with the placeholder text that won't be useful. Instead please update the description to be verbose with the full info that these fields need to be filled in if undetected.

You could potentially add secondary fields which auto populate as to whether the fields were automatically detectable or had to be manually filled out.
I think that something to this effect for both could be clearer.

  • Able to detect referrer autocratically if no, please fill in the referrer url below, if yes please verify the url looks correct below: [ yes, no]
  • Referrer url: ""

@gabrielhsdev
Copy link
Author

Hi @tfoote
I'm about to start implementing the requested changes. From what I gathered, I need to:

  • Integrate a checkbox (yes or no) for users to signal whether the referrer/404 URL is properly filled, complete with a concise description.
  • Eliminate the placeholder text (based on my personal tests, it seems unlikely to pass validation).
  • Update description to be verbose with the full info that these fields need to be filled in if undetected.

Does this align with your expectations?

Moreover, it's worth noting that the placeholder text doesn't auto-populate the form. Instead, it functions as a visual cue, presenting text within the input field until users input their own text, which then supersedes the placeholder. Essentially, it serves as a subtle prompt rather than pre-filling the form.

Please refer to the image below: the 'Submit New Issue' button remains unavailable until both required fields are properly filled.

image

.github/ISSUE_TEMPLATE/form404.yml Outdated Show resolved Hide resolved
.github/ISSUE_TEMPLATE/form404.yml Outdated Show resolved Hide resolved
.github/ISSUE_TEMPLATE/form404.yml Outdated Show resolved Hide resolved
.github/ISSUE_TEMPLATE/form404.yml Outdated Show resolved Hide resolved
.github/ISSUE_TEMPLATE/form404.yml Outdated Show resolved Hide resolved
@tfoote
Copy link
Member

tfoote commented Apr 2, 2024

Thanks for the testing on your instance and finding out that the validation works better than I had expected. I'll give it a try with slightly tweaked verbiage.

@gabrielhsdev
Copy link
Author

gabrielhsdev commented Apr 2, 2024

Thanks @tfoote

Based on my experimentation, it appears that the required fields function solely on public repositories. The following quote provides further insight: Prevents form submission until element is completed. Only for public repositories.

Should I add the checkboxes ?

Thanks

@tfoote tfoote merged commit f80416e into ros-infrastructure:ros2 Apr 2, 2024
1 check passed
@tfoote
Copy link
Member

tfoote commented Apr 2, 2024

With the placeholders not filling the validation it's not necessary.

Trying it out https://github.com/ros-infrastructure/rosindex/issues/new?labels=bug&template=form404.yml&referrer=testreferrerURL&404page=test404pageurl&title=test_title

I found one bug that there's a typo in the referrer key which is missing an r https://github.com/ros-infrastructure/rosindex/pull/365/files#diff-128cc35baaf40bac8ae4d2932ca6e009311e8279f03f258e9cc83966f1d5b32eR64

tfoote added a commit that referenced this pull request Apr 2, 2024
Fixing referrer url tag string from #365
@tfoote tfoote mentioned this pull request Apr 2, 2024
tfoote added a commit that referenced this pull request Apr 2, 2024
Fixing referrer url tag string from #365
@gabrielhsdev
Copy link
Author

gabrielhsdev commented Apr 2, 2024

Thank you @tfoote.
I'll check out other issues to see if I can offer any assistance.
If I can be of help somehow, you could send me an email at or maybe just tag me on an issue.
Email: henriquegabrielhs21@gmail.com

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants